Histogram implementation cleanup #2283
Conversation
fraillt
force-pushed
the
histogram-impl-cleanup
branch
from
fc5475a to
e0b962a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2283 +/- ##
=====================================
Coverage 79.4% 79.4%
=====================================
Files 122 122
Lines 20795 20783 -12
=====================================
- Hits 16512 16506 -6
+ Misses 4283 4277 -6 ☔ View full report in Codecov by Sentry. |
fraillt
force-pushed
the
histogram-impl-cleanup
branch
from
6b422d5 to
d45b5ac
Compare
| let buckets_count = boundaries.len() + 1; | ||
| let mut histogram = Histogram { | ||
| pub(crate) fn new(mut bounds: Vec<f64>, record_min_max: bool, record_sum: bool) -> Self { | ||
| bounds.retain(|v| !v.is_nan()); |
There was a problem hiding this comment.
There are only 3 ways bounds can come from:
- Default. - In this case, sdk provides it so we ensure its already sorted and validated.
- HistogramCreation API - In this case, user provides it, but while creating the instrument, sdk should validate it, and ensure its sorted/validated. If the validation fails, sdk can return the defaults or noop instrument. Opened this issue to track it: Validate user provided Histogram bounds at sdk during Instrument creation time #2286
- Views - In this case also, user provides it, but we should do same as 2.
I believe this method is the wrong place to validate/filter bounds as it should be taken care elsewhere.
Not a blocker for this PR, just added a comment to make sure have same understanding.
There was a problem hiding this comment.
I believe this method is the wrong place to validate/filter bounds as it should be taken care elsewhere.
💯
cijothomas
left a comment
cijothomas
left a comment
There was a problem hiding this comment.
LGTM!
Left some comments but not a blocker at all.
3 participants
Changes
Mostly refactoring by removing unnecessary code, with few minor improvements:
.clone()on buckets indeltacollection.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes